logpuller: split subscription runtime helpers from subscription client by lidezhu · Pull Request #5603 · pingcap/ticdc

lidezhu · 2026-07-09T06:37:05Z

What problem does this PR solve?

Issue Number: close #5604

What is changed and how it works?

This pull request refactors the log puller's subscription client to improve modularity and maintainability. By decomposing the monolithic subscription client into specialized components, the system gains better separation of concerns regarding event processing, error recovery, and span lifecycle management. These changes facilitate more robust handling of region events and failures while simplifying the codebase for future development.

Highlights

Component Refactoring: Restructured the subscription client by extracting event delivery, failure handling, and span lifecycle management into three dedicated components: regionEventSink, regionFailureHandler, and spanRegistry.
Improved Reliability: Introduced centralized span tracking and improved recovery logic for region failures, enhancing the stability of log puller subscriptions.
Enhanced Monitoring: Updated metrics collection to provide better visibility into dynamic stream memory usage and resolved timestamp lag.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Questions

Will it cause performance regression or break compatibility?

Do you need to update user documentation, design documentation or monitoring documentation?

Release note

Please refer to [Release Notes Language Style Guide](https://pingcap.github.io/tidb-dev-guide/contribute-to-tidb/release-notes-style-guide.html) to write a quality release note.

If you don't think this PR needs a release note then fill it with `None`.

Summary by CodeRabbit

New Features
- Added a revamped region event and failure handling pipeline for log puller subscriptions.
- Introduced centralized span tracking to improve resolved-ts and region monitoring metrics.
Bug Fixes
- Improved recovery routing for region failures (including retries and follow-up requests).
- Enhanced behavior during paused/resumed event delivery.
Refactor
- Reworked subscription internals to separate event delivery, failure handling, and span management.
Tests
- Added/updated unit tests for pause/resume, metrics updates, and span registry behavior.

ti-chi-bot · 2026-07-09T06:37:08Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

coderabbitai · 2026-07-09T06:37:17Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bc43778b-a434-4197-a0c0-03470185d574

📥 Commits

Reviewing files that changed from the base of the PR and between 12232a4 and 125b52b.

📒 Files selected for processing (5)

logservice/logpuller/region_event_sink.go
logservice/logpuller/region_event_sink_test.go
logservice/logpuller/region_failure_handler.go
logservice/logpuller/span_registry.go
logservice/logpuller/span_registry_test.go

🚧 Files skipped from review as they are similar to previous changes (3)

logservice/logpuller/span_registry.go
logservice/logpuller/region_failure_handler.go
logservice/logpuller/region_event_sink.go

📝 Walkthrough

Walkthrough

The log puller extracts dynamic-stream event delivery, region failure handling, and subscribed-span lifecycle management from subscriptionClient into regionEventSink, regionFailureHandler, and spanRegistry, updating runtime wiring and tests.

Changes

Subscription client decomposition

Layer / File(s)	Summary
regionEventSink: dynstream event delivery `logservice/logpuller/region_event_sink.go`, `logservice/logpuller/region_event_sink_test.go`	Adds pause-aware event delivery, feedback handling, subscription controls, metrics, shutdown behavior, and corresponding tests.
regionFailureHandler: error buffering and recovery `logservice/logpuller/region_failure_handler.go`	Adds failure reporting, batched error dispatch, categorized error handling, retry scheduling, and table-drain handling.
spanRegistry: span lifecycle and monitoring `logservice/logpuller/span_registry.go`, `logservice/logpuller/span_registry_test.go`	Adds subscribed-span ownership, registry operations, resolve-lock maintenance, slow-region logging, and metrics.
Client and handler rewiring `logservice/logpuller/subscription_client.go`, `logservice/logpuller/region_event_handler.go`	Delegates subscription, event, failure, metrics, runtime, and shutdown paths to the new components.
Test wiring updates `logservice/logpuller/subscription_client_test.go`, `logservice/logpuller/region_request_worker_test.go`	Updates test construction and assertions for the extracted components.

Estimated code review effort: 4 (Complex) | ~60 minutes

Suggested labels: lgtm

Suggested reviewers: asddongmen, wk989898

Poem

A rabbit split the burrow wide,
Three tidy tunnels now abide—
Events hop, failures flow,
Spans keep watch where regions grow.
🐇 Cleaner paths through every node!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main refactor: splitting subscription runtime helpers out of the client.
Linked Issues check	✅ Passed	The PR extracts event sink, failure handling, and span registry as requested while preserving the subscription client behavior.
Out of Scope Changes check	✅ Passed	The changes stay within the refactor scope and add only supporting tests and internal plumbing.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch ldz/refactor-puller01

Warning

Tools execution failed with the following error:

Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error)

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

gemini-code-assist

Code Review

This pull request refactors the logpuller package by modularizing subscriptionClient and extracting its responsibilities into dedicated components: regionEventSink for managing event streams, regionFailureHandler for error dispatching, and spanRegistry for tracking subscribed spans. Additionally, a pullerMemoryQuota is introduced to handle memory backpressure. Feedback on these changes highlights a missing memoryQuota field and initialization in subscriptionClient that would cause compilation errors and nil pointer dereferences. The review also recommends cleaning up unused fields and methods in errCache, and refactoring the sync.Cond synchronization in regionEventSink to a safer, channel-based signaling mechanism to prevent goroutine leaks during context cancellation.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

coderabbitai

🧹 Nitpick comments (1)

logservice/logpuller/span_registry.go (1)
274-312: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

logSlowRegions holds RLock across IterAll + logging for all spans.

Unlike runResolveLockChecker (which snapshots into spanAndTsCache and releases the lock before doing work), this loop performs rangeLock.IterAll(nil) and emits log.Info for every span while still holding r.RLock(). With many spans this can stall Add/Remove (i.e. Subscribe/Unsubscribe) for the duration of the scan. Consider snapshotting the spans (or their attrs) under the read lock and logging outside it, mirroring the resolve-lock checker.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@logservice/logpuller/span_registry.go` around lines 274 - 312, The
`logSlowRegions` loop in `spanRegistry` is holding `r.RLock()` while calling
`rangeLock.IterAll(nil)` and emitting multiple `log.Info` entries, which can
block `Subscribe`/`Unsubscribe` for the full scan. Update `logSlowRegions` to
snapshot the needed span/attr data under the read lock, then release the lock
before doing the time checks and logging, following the same pattern used by
`runResolveLockChecker` and its `spanAndTsCache`.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@logservice/logpuller/span_registry.go`:
- Around line 274-312: The `logSlowRegions` loop in `spanRegistry` is holding
`r.RLock()` while calling `rangeLock.IterAll(nil)` and emitting multiple
`log.Info` entries, which can block `Subscribe`/`Unsubscribe` for the full scan.
Update `logSlowRegions` to snapshot the needed span/attr data under the read
lock, then release the lock before doing the time checks and logging, following
the same pattern used by `runResolveLockChecker` and its `spanAndTsCache`.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 069e02a2-3506-4694-8d51-b16be08fedd8

📥 Commits

Reviewing files that changed from the base of the PR and between 040cb1e and 12232a4.

📒 Files selected for processing (7)

logservice/logpuller/region_event_handler.go
logservice/logpuller/region_event_sink.go
logservice/logpuller/region_failure_handler.go
logservice/logpuller/region_request_worker_test.go
logservice/logpuller/span_registry.go
logservice/logpuller/subscription_client.go
logservice/logpuller/subscription_client_test.go

lidezhu · 2026-07-09T10:45:27Z

/gemini summary

gemini-code-assist · 2026-07-09T10:45:40Z

Summary of Changes

This pull request refactors the log puller's subscription client to improve modularity and maintainability. By decomposing the monolithic subscription client into specialized components, the system gains better separation of concerns regarding event processing, error recovery, and span lifecycle management. These changes facilitate more robust handling of region events and failures while simplifying the codebase for future development.

Highlights

Component Refactoring: Restructured the subscription client by extracting event delivery, failure handling, and span lifecycle management into three dedicated components: regionEventSink, regionFailureHandler, and spanRegistry.
Improved Reliability: Introduced centralized span tracking and improved recovery logic for region failures, enhancing the stability of log puller subscriptions.
Enhanced Monitoring: Updated metrics collection to provide better visibility into dynamic stream memory usage and resolved timestamp lag.

New Features

🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Activity

The pull request was created and marked as a draft.
Automated code review identified several critical and medium-priority issues, including missing fields, potential nil pointer dereferences, and anti-patterns in concurrency control.
The author requested a summary of the changes.
Review comments provided specific suggestions for fixing the identified issues, such as using channel-based signaling instead of sync.Cond and cleaning up unused error cache fields.

lidezhu · 2026-07-09T10:46:57Z

/test all

ti-chi-bot · 2026-07-10T08:52:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: asddongmen, hongyunyan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [asddongmen,hongyunyan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2026-07-10T08:52:27Z

[LGTM Timeline notifier]

Timeline:

2026-07-10 03:32:29.157599776 +0000 UTC m=+338935.193694842: ☑️ agreed by asddongmen.
2026-07-10 08:52:25.986959465 +0000 UTC m=+358132.023054531: ☑️ agreed by hongyunyan.

lidezhu · 2026-07-10T09:18:48Z

/test pull-unit-test-next-gen

logpuller: split subscription runtime helpers from subscription client

41d1081

ti-chi-bot Bot added do-not-merge/needs-linked-issue do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 9, 2026

ti-chi-bot Bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed do-not-merge/needs-linked-issue labels Jul 9, 2026

gemini-code-assist Bot reviewed Jul 9, 2026

View reviewed changes

lidezhu added 6 commits July 9, 2026 15:32

remove unnecessary file

c27f91d

small refactor

f83181f

more refactor

cf031f8

refactor

13d997f

fix

8e0ba23

refactor

12232a4

lidezhu marked this pull request as ready for review July 9, 2026 10:17

ti-chi-bot Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 9, 2026

coderabbitai Bot reviewed Jul 9, 2026

View reviewed changes

add some test

f2167b3

asddongmen approved these changes Jul 10, 2026

View reviewed changes

ti-chi-bot Bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Jul 10, 2026

address comment

125b52b

hongyunyan approved these changes Jul 10, 2026

View reviewed changes

ti-chi-bot Bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jul 10, 2026

ti-chi-bot Bot merged commit 1971d2b into master Jul 10, 2026
22 of 24 checks passed

ti-chi-bot Bot deleted the ldz/refactor-puller01 branch July 10, 2026 09:42

Uh oh!

Conversation

lidezhu commented Jul 9, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Highlights

Check List

Tests

Questions

Will it cause performance regression or break compatibility?

Do you need to update user documentation, design documentation or monitoring documentation?

Release note

Summary by CodeRabbit

Summary by CodeRabbit

Uh oh!

ti-chi-bot Bot commented Jul 9, 2026

Uh oh!

coderabbitai Bot commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

lidezhu commented Jul 9, 2026

Uh oh!

gemini-code-assist Bot commented Jul 9, 2026

Summary of Changes

Highlights

Uh oh!

lidezhu commented Jul 9, 2026

Uh oh!

ti-chi-bot Bot commented Jul 10, 2026

Uh oh!

ti-chi-bot Bot commented Jul 10, 2026

[LGTM Timeline notifier]

Uh oh!

lidezhu commented Jul 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lidezhu commented Jul 9, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 9, 2026 •

edited

Loading